-
Notifications
You must be signed in to change notification settings - Fork 218
Reschedule delete #600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reschedule delete #600
Conversation
Bumps [jandex-maven-plugin](https://github.com/wildfly/jandex-maven-plugin) from 1.1.1 to 1.2.1. - [Release notes](https://github.com/wildfly/jandex-maven-plugin/releases) - [Commits](wildfly/jandex-maven-plugin@1.1.1...1.2.1) --- updated-dependencies: - dependency-name: org.jboss.jandex:jandex-maven-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [mockito-core](https://github.com/mockito/mockito) from 3.12.4 to 4.0.0. - [Release notes](https://github.com/mockito/mockito/releases) - [Commits](mockito/mockito@v3.12.4...v4.0.0) --- updated-dependencies: - dependency-name: org.mockito:mockito-core dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This is a major change and backbone of v2. We change also how the Resources are addressed both internally and from event source with CustomResourceID. Additional improvements also added.
# Conflicts: # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from the minor naming issues I have 😄
|
||
private Long reScheduleDelay = null; | ||
|
||
public T withReSchedule(long delay) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename this to reschedulingAfter
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yep, maybe rescheduleAfter(...)
without -ing
?
return withReSchedule(timeUnit.toMillis(delay)); | ||
} | ||
|
||
public Optional<Long> getReScheduleDelay() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ReSch...
looks weird to me 😅 Would simply getDelay
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just getScheduleDelay()
? the BaseController
name does not tell anything about the rescheduling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG naming is hard :D
import java.util.Optional; | ||
import java.util.concurrent.TimeUnit; | ||
|
||
public abstract class ControlBase<T extends ControlBase> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda nit picking but since the subclasses are named UpdateControl
and DeleteControl
, would it make more sense to name that class BaseControl
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, will change it. Probably it comes to my mind from Spring, I remember there these "base" suffxed classes.
@metacosm how does it look now? :) |
* chore: renaming vars named k8sClient to kubernetsClient * chore(deps): bump jandex-maven-plugin from 1.1.1 to 1.2.1 (#592) Bumps [jandex-maven-plugin](https://github.com/wildfly/jandex-maven-plugin) from 1.1.1 to 1.2.1. - [Release notes](https://github.com/wildfly/jandex-maven-plugin/releases) - [Commits](wildfly/jandex-maven-plugin@1.1.1...1.2.1) --- updated-dependencies: - dependency-name: org.jboss.jandex:jandex-maven-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump mockito-core from 3.12.4 to 4.0.0 (#591) Bumps [mockito-core](https://github.com/mockito/mockito) from 3.12.4 to 4.0.0. - [Release notes](https://github.com/mockito/mockito/releases) - [Commits](mockito/mockito@v3.12.4...v4.0.0) --- updated-dependencies: - dependency-name: org.mockito:mockito-core dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feature: Build PR on v2 * chore(ci): use Java 17 * chore(ci): use only Temurin distribution * chore: add generics to PostExecutionControl to reduce IDEs noise (#594) * chore: polish the junit5 extension (#593) * feat: Use informers as CustomResourceEventSource backbone and cache This is a major change and backbone of v2. We change also how the Resources are addressed both internally and from event source with CustomResourceID. Additional improvements also added. * feat: delete reschedule * fix: naming after review Co-authored-by: Ioannis Canellos <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chris Laprun <[email protected]> Co-authored-by: Luca Burgazzoli <[email protected]>
No description provided.